Skip to content

Conversation

@enjoy-binbin
Copy link
Member

The client eviction introduced in 2753429,
at that time, the main idea of our introduction was to protect key data
from being evicted, which is the server's perspective.

In fact, the accumulated client output buffer can also violate the maxmemory
contract and causing the machine to OOM.

…default

The client eviction introduced in 2753429,
at that time, the main idea of our introduction was to protect key data
from being evicted, which is the server's perspective.

In fact, the accumulated client output buffer can also violate the maxmemory
contract and causing the machine to OOM.

Signed-off-by: Binbin <[email protected]>
@enjoy-binbin enjoy-binbin added the major-decision-pending Major decision pending by TSC team label Mar 12, 2025
@codecov
Copy link

codecov bot commented Mar 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.04%. Comparing base (bcd2f95) to head (ac08340).

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1840      +/-   ##
============================================
+ Coverage     70.97%   71.04%   +0.06%     
============================================
  Files           123      123              
  Lines         65665    65665              
============================================
+ Hits          46608    46649      +41     
+ Misses        19057    19016      -41     
Files with missing lines Coverage Δ
src/config.c 78.77% <ø> (+0.38%) ⬆️

... and 11 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hwware
Copy link
Member

hwware commented Mar 12, 2025

I am not sure if 100% as default value is reasonable

Copy link
Collaborator

@hpatro hpatro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shouldn't enable it by default. It cause throughput degradation (extra cpu cycle during command processing) due to constant tracking/metric update of client's input/output buffer size during read/write flow.

redis/redis#11348

@zuiderkwast
Copy link
Contributor

zuiderkwast commented Mar 31, 2025

We discussed this briefly before we released 8.0. Here is the discussion: #653 (comment).

My thoughts:

  • Clients using 100% of the configured maxmemory is very unlikely. If it happens, then we have other serious problems: It means all keys are being evicted or are already evicted, or if key eviction is disabled, we are already far over maxmemory and clients receive the OOM error.
  • I'm concerned about the performance degradation raised by @hpatro.
  • A better value would be 50% or 10% but we can't use that as the default value because it would be a breaking change. I think users who want client eviction need to configure it themselves.

@hwware
Copy link
Member

hwware commented Apr 1, 2025

We discussed this briefly before we released 8.0. Here is the discussion: #653 (comment).

My thoughts:

  • Clients using 100% of the configured maxmemory is very unlikely. If it happens, then we have other serious problems: It means all keys are being evicted or are already evicted, or if key eviction is disable, we are already far over maxmemory and clients receive the OOM error.
  • I'm concerned about the performance degradation raised by @hpatro.
  • A better value would be 50% or 10% but we can use that as the default value because it would be a breaking change. I think users who want client eviction need to configure it themselves.

I prefer to leave it and let client decide the default value.

@madolson
Copy link
Member

madolson commented Apr 7, 2025

Consensus from our weekly meeting was that we aren't entirely clear what problem this is trying to solve. There were concerns about folks migrating from Redis seeing different performance. So the preference was to not change the default and let users decide. This seems aligned with what other folks have said in this thread.

@enjoy-binbin enjoy-binbin removed this from Valkey 9.0 Jul 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

major-decision-pending Major decision pending by TSC team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants